-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ft2mne #25
ft2mne #25
Conversation
…art for the reading function
…the corresponding reading functions
That would be great! I have a few quick thoughts that make me pause a bit though:
What do you think? Would you be up for trying (3) in a separate PR then we merge it and come back to this? (Or you could add to this PR if you want, but it would make me a bit less comfortable from a not-breaking-what-we-have-currently standpoint...) Would you be willing to help maintain this module a bit going forward? I'm not saying 100% that these things are required, but if we could go that route it's much safer than merging this with fingers crossed (after pretty lax reviewing) that everything is okay... cc @britta-wstnr who I mentioned these concerns/ideas to today, feel free to chat with her if you want, we can discuss here more, or you could hop on to office hours a week from today and we could discuss over video (might be easier)! |
cc @ftadel |
Thanks for your thoughtful reply Eric! Actually, I already started an answer to your comments earlier today, but somehow I forgot to press the 'comment' button, so the first attempt is now lost to posterity. A bit less well formulated in a second attempt, then. I understand your concerns, and would be game to provide some test functionality. I have already started this (of course) on the FieldTrip end, and so far focussed on not too fine grained unit tests (basically checking if the overall writing/reading works at all, and whether we get out what we expect). We have our own pet datasets to test with, but @britta-wstnr pointed me to your mne-testing-data repo, which I can also look at. I will have to think a bit about how easy it would be for me to provide a set of tests for the current functionality in a separate PR, because I need some of the proposed changes to the low-level code to get the intermediate/higher level writing/reading up-and-running to begin with. Would it be convincing enough to you if I were able to show (on all the 'raw' fif-files in mne-testing-data repo) that I can load them into matlab (using FieldTrip, which for the low-level stuff uses the code from mne-matlab) and write them back into a fif-file (using FieldTrip), conserving the time series data (at single precision)? I think that this is functionality that should work before I started tampering with fieldtrip2fiff and the mne code. Anything more fancy than that (e.g. reading evoked data without troubles, writing epoched data) already would require some upgraded code. I could submit this type of test in a separate PR, for your peace of mind, no problem. I haven't touched any functionality other than pertaining to the reading of raw/epoched/evoked data, or the writing of events, so I am not sure whether I can contribute to any test functions there (yet). I wouldn't mind taking some responsibility in maintaining this repo, no problem. |
At the end of the day I just want to minimize the likelihood of breaking stuff for people, and the time it takes to un-break it when we (inevitably) accidentally do break it. But I also don't want to stand in the way of much overdue progress here. So if it's a lot easier for you to put tests in this PR for what you've added -- rather than starting with a separate PR to add tests for the code we already have -- that's okay with me, too, especially since you've volunteering to fix bugs you introduce :)
I didn't put in any mechanisms to download/extract/use the testing data. For now maybe just start from these files, which are part of the this git repo: https://github.com/mne-tools/mne-matlab/tree/master/matlab/test/data When you need more files than these (maybe that's already the case?) then we can think about integrating mne-testing-data or some other datasets as needed. It would be better not to add them to this repo in the future, though, if possible. Repo bloat becomes a problem with too many binaries added... In any case, here is what I did for tests in the PR where I added the infrastructure: You can see I ignored my own advice and added a file to the repo 🤭 |
Ah, of course, this is a different repo ... 🤦 so that was bad advice, sorry @schoffelen ! |
Well, not too bad actually. I have to get used to the (better than in FieldTrip) practice of unit testing, which requires stuff to be more self-contained. For @larsoner 's background: for FieldTrip we always run a series of (extensive) tests overnight on our local file system, where we can exhaustively use a large amount of data. I just installed Moxunit, and will look into this for the current PR. (PS: any advice regarding the distinction between fiff_ and mne_ functions? Our (i.e. FT's) aim has so far been to use fiff_ functions, which appear the more 'original' ones) |
Yes I think the |
AFAIK fiff_ function are almost line by line reimplementation of fif
library C code while mne_ functions are highler level and use the fiff_
functions
… Message ID: ***@***.***>
|
Hi @larsoner I was wondering whether you would feel inclined to do a quick review of this PR, if not, I'd be happy to merge without further ado. I think I have added a few key tests that evaluate the new (and some of the old) functionality. Once this is out of the way, I'd like to sync back into FieldTrip, and then create a new PR from a fresh master branch, that incorporates some speed improvements for the reading of one-tag-per-datasample fif-files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me, just one idea about removing old code!
assertEqual(evoked.evoked, evokednew.evoked); | ||
|
||
% clean up | ||
delete(fnamenew); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooray unit tests 🎉
Hi all, specifically @britta-wstnr and @larsoner I was hoping whether the proposed changes in this PR resonate with you. I am aiming for a more seamless bridge between FieldTrip and MNE-Python (from the FT-end) and have some improvement suggestions for the matlab-based writing and reading code for fif-files. I know that most of the code is considered kind of legacy, but I noticed a few loose ends here and there, which inspired me to make some suggestions for improved consistency, e.g. w.r.t. having a symmetric pair of writing/reading functions with matching functionality, so that it is possible to make a round trip. For now, the suggested changes are in the fieldtrip-repo, because they are needed for my recently merged improved version of the fieldtrip2fiff function. The end goal here would be to have an FT-based equivalent of the mne.io.read_epochs/evoked/raw_fieldtrip functionality, but ideally with the info-info transmitted as reliably as possible